Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Trigger eagerloading for first() and last() #10875

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Jul 19, 2023

The new logic is directly pulled from SQLSelect::firstRow() and SQLSelect::lastRow() which is was ultimately was powering these methods - tracing those, you can see that this limited query (limit 1) was always being run, and the result (which was necessarily an array of size one) was returned from the database - and then just the first and last rows respectively were pulled out from there in PHP.

So the logic is unchanged, I'm just pulling it up to this level so we can also trigger eager loading - and only trigger it once. i.e. calling first() then last() and then iterating over the same list won't trigger multiple db queries.

Issue

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests should add 3x records each with different titles and then confirm that ->first() and ->last() have returned the correct objects

@GuySartorelli
Copy link
Member Author

I had assumed DataListTest would already have that - but it doesn't! Which is worrying, but I'll add that now.

@GuySartorelli GuySartorelli force-pushed the pulls/5/fix-eagerloaded-firstlast branch from eee426e to 02ae5a4 Compare July 20, 2023 02:24
@GuySartorelli GuySartorelli force-pushed the pulls/5/fix-eagerloaded-firstlast branch from 02ae5a4 to df2a02e Compare July 20, 2023 03:26
@emteknetnz emteknetnz merged commit 642321d into silverstripe:5 Jul 20, 2023
@emteknetnz emteknetnz deleted the pulls/5/fix-eagerloaded-firstlast branch July 20, 2023 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants